/claude-review

Code Review

Summary

This PR implements MSM (Multi-Scalar Multiplication) as a black-box opcode in the R1CS compiler, dispatching Noir's embedded_curve_ops::multi_scalar_mul to a custom constraint system rather than the native Noir implementation. The core idea is a two-base FakeGLV decomposition: for each input (P, s), the prover supplies R = s·P via hint, then verifies |s1|·P_eff + |s2|·R_eff = 0 (Phase 2) and the scalar relation ±|s1| ± |s2|·s ≡ 0 (mod n) (Phase 3). Together these two checks soundly establish R = s·P per point. Phase 4 accumulates per-point results into the final MSM output.

Review

Correctness & Soundness

The overall proof strategy is sound. Phases 2 + 3 jointly prove R = s·P for each point without directly inverting s2 in-circuit. Specific checks:

• compute_is_zero is correctly implemented using the standard two-constraint gadget (v·v_inv = 1 - is_zero, v·is_zero = 0). The output is uniquely determined and boolean without needing an explicit constrain_boolean. ✓
• inf_flag is boolean-constrained via constrain_boolean in detect_skip. ✓
• neg1/neg2 from FakeGLVHint are implicitly constrained boolean via ops.select(neg1_witness, ...) / ops.select(neg2_witness, ...) in preprocess_points, which internally calls constrain_boolean. ✓
• s2 ≠ 0 is enforced via constrain_zero(ops.compiler, s2_is_zero) in verify_scalar_relation — critical to prevent the degenerate case where the relation degenerates and leaves R unconstrained. ✓
• On-curve checks are applied to both the sanitized input point and the hint-output R. ✓
• The select_unchecked / point_select_unchecked calls all operate on flags that are provably boolean from prior constraints. ✓
• Accumulation uses the offset trick to avoid hitting the identity or requiring a doubling; per-point offset verification is done independently. ✓

One soundness concern to flag:

neg1/neg2 boolean check ordering. emit_fakeglv_hint is called before ops.select(neg1_witness, ...). If any future refactor moves the select call after the scalar_relation::verify_scalar_relation call (which uses neg1/neg2 with select_unchecked-equivalent operations), the boolean constraint on neg1/neg2 would disappear. The existing code is correct, but the boolean constraint is load-bearing and somewhat implicit. Consider adding an explicit constrain_boolean for neg1 and neg2 after emit_fakeglv_hint so the invariant is self-contained and not order-dependent.

Code Quality

Blocking: .expect() in library code — CLAUDE.md prohibits .unwrap() / .expect() in library code. Three violations:

1. provekit/r1cs-compiler/src/msm/ec_points/hints_native.rs — FieldElement::from_bigint(ark_ff::BigInt(params.ec.curve_a_raw)).expect("curve_a must fit in native field") (appears twice: once in point_double_verified_native, once in verify_on_curve_native). These curve parameters are known at compile time for supported curves and will never overflow BN254, but library code must still return anyhow::Result and propagate via ?. Consider encoding the conversion as a static assertion or moving parameter validation to EcFieldParams construction.

2. provekit/r1cs-compiler/src/msm/pipeline.rs in preprocess_points — all_skipped.expect("MSM must have at least one point"). Should use anyhow::ensure!(n_points > 0, "MSM must have at least one point") at the start of process_multi_point instead, propagating the error up.

Minor: Missing /// doc comments on pub methods in multi_limb_ops.rs. The add, sub, and mul methods on MultiLimbOps have #[must_use] but no /// doc comments. CI enforces cargo doc -D warnings for all pub items.

Non-blocking: NoirToR1CSCompiler and add_range_checks visibility widened to pub. Both were pub(crate). The change to pub broadens the public API of the r1cs-compiler crate. This appears intentional (the bench test uses them), but warrants a comment or #[doc(hidden)] if they are not intended as stable API.

Non-blocking: No comment on Grumpkin hardcode. In noir_to_r1cs.rs, MultiScalarMul always dispatches to crate::msm::curve::Grumpkin. A single-line comment // Noir's embedded_curve_ops::multi_scalar_mul targets the Grumpkin curve embedded in BN254 would clarify the intent.

Testing

• Grumpkin (native field, single-limb) path is tested end-to-end via embedded_curve_msm example including several degenerate cases (zero scalar, infinity, near-identity, near-order). ✓
• msm_witness_solving.rs provides detailed unit tests for the FakeGLV decomposition and witness solving. ✓
• The non-native path (NonNativeEcOps, multi-limb, e.g. secp256r1) has no integration test. The hints_non_native.rs file is ~462 lines of schoolbook column equation logic and contributes to a significant portion of added complexity. At minimum a property-based test verifying round-trip (prove + verify) for the secp256r1 curve would be expected before merge.
• No regression test for the .expect() crash paths (e.g., calling add_msm_with_curve with zero points). Once fixed to return anyhow::Result, the error path should be tested.

Suggestions (non-blocking)

• The bigint_mod.rs module in provekit/prover re-implements divmod, mod_pow, half_gcd, etc. entirely from scratch using 4-limb arithmetic. Consider whether num-bigint (already added to workspace deps) or ark-ff's BigInt primitives could replace some of this to reduce the maintenance surface. In particular, half_gcd is a non-trivial algorithm; a reference or proof-of-correctness comment would help future reviewers.
• scripts/verify_offset_points.py is a useful sanity-check script. Consider adding a comment at the top of curve/grumpkin.rs and curve/secp256r1.rs pointing to this script for the precomputed offset/accumulated-offset values.

Verdict

🔄 Request changes — The .expect() calls in library code are a CLAUDE.md violation and must be fixed. The missing integration test for the non-native path is a notable testing gap for safety-critical code. Everything else can be addressed as follow-ups.

Avoid use of .unwrap(), either use .expect("") or float the error with Result.